fix(logger): correctly refresh sample rate #3722
Conversation
boring-cyborg
bot
added
logger
boring-cyborg
bot
added
the
documentation
There was a problem hiding this comment.
Thanks for the quick fix of bug. I am still a bit puzzled about the cause and also the solution. I see that we have now two fields to capture sampleRate value but use only one of the for the refresh decision.
You have also decoupled the coldStart into a dedicated counter to check if we are in a warm state or now.
Can you elaborate on the solution? Is that correct that refreshSampleRate is skipped because of the order we read env variable last and because we are passing a cached constructor value, which is 0?
|
|
The previous implementation was checking the cold start in two places:
Only the first one flipped the state, but when refreshing the sample rate manually and not using the context (as the example in the linked issue) the state never got flipped and so the method always returned early. You can verify this by pulling the main branch and adding a test that does this:
If you debug the test and step through (or just put a breakpoint in the method) you'll see it never calls the sample rate refresh internally. Either way, in hindsight, we shouldn't have relied on the set initial sample rate method to refresh it either, since there's a lot of logic that we don't need to run every time. Now it's the other way around, and we keep a dedicated state for the sample rate config: one to keep track of the actual rate (which was stored in a nested object that didn't belong to it) and another to keep track of how many times it's been called. |
Summary
Changes
This PR refactors the internal logic to calculate and refresh log sampling so that it works for any usage pattern. In #3278 we introduced changes that automatically refresh the sample rate calculation when using the
injectLambdaContext()class method decorator and Middy.js middleware, but broke the manual method in the process.The PR fixes the bug reported by a customer in the linked issue.
Issue number: fixes #3718
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.